Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comments block: Remove empty block wrapper #43383

Merged
merged 4 commits into from
Aug 19, 2022

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Aug 18, 2022

What?

If posting comments is disabled for a given post (and the post doesn't have any comments yet), we used to render an empty <div class="wp-comments"> </div> block wrapper. This PR removes that empty wrapper.

This fixes #41957. (Note that the block used to be called "Comments Query Loop" at the time but has since been renamed to just "Comments".)

Why?

The empty wrapper would get in the way of some layouts, see #41957.

How?

Moving the check that bails early upon no comments further up in the block's index.php.

Testing Instructions

  1. Publish a page or post
  2. In the Post sidebar, uncheck "Allow comments"
  3. View front end

Screenshots or screencast

Before After
image image

@ockham ockham added the [Block] Comments Affects the Comments Block - formerly known as Comments Query Loop label Aug 18, 2022
@ockham ockham self-assigned this Aug 18, 2022
@ockham ockham marked this pull request as ready for review August 18, 2022 13:33
@ockham ockham force-pushed the fix/empty-comments-block-wrapper branch from 8fbb549 to dad7549 Compare August 18, 2022 14:36
@ockham
Copy link
Contributor Author

ockham commented Aug 19, 2022

I'll try to add a small unit test maybe.

@ockham ockham marked this pull request as draft August 19, 2022 09:38
@ockham ockham marked this pull request as ready for review August 19, 2022 09:53
Copy link
Contributor

@cbravobernal cbravobernal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

In order for me to work, I had to remove and add again the Comments block on the Site Editor, is this a correct behavior?

Apart from that, it looks good to me (thanks for adding the clarifying comment in L42!

@DAreRodz
Copy link
Contributor

In order for me to work, I had to remove and add again the Comments block on the Site Editor, is this a correct behavior?

@c4rl0sbr4v0 , I would expect that, but I know it's not ideal.

I think that happened because the block type was core/comments-query-loop, and that type is only replaced with core/comments on the editor, so you would have to save changes first there in order to update the block.

It should work fine with the old core/post-comments without removing the block, though, as we are unregistering the block type definition and registering a new one with a different render callback (see this). Although, in that case, it would render the fallback version which doesn't add any markup. 😅

Would it make sense to do something similar (I mean the unregister/register thing) for core/comments-query-loop? 🤔

cc: @ockham

@ockham
Copy link
Contributor Author

ockham commented Aug 19, 2022

Thanks a lot for testing, @c4rl0sbr4v0 and @DAreRodz!

In order for me to work, I had to remove and add again the Comments block on the Site Editor, is this a correct behavior?

Hmm, I didn't really expect that, TBH 🤔

I think that happened because the block type was core/comments-query-loop, and that type is only replaced with core/comments on the editor, so you would have to save changes first there in order to update the block.

Hmm, I'm not sure that's accurate:

  1. The Site Editor is supposed to apply legacy block conversions (and deprecations) right upon loading. (I know it didn't seem to do that for a while, but I believe that's fixed per Site Editor: Make Code Editor reflect block conversions #42081.)
  2. If Carlos was using Twenty Twenty-Two to test -- that theme uses the Post Comments block (rather than Comments Query Loop).

Is it possible that Gutenberg hadn't finished rebuilding when you were testing this? (Since this is block PHP, it actually has to go through the build step.)

@ockham
Copy link
Contributor Author

ockham commented Aug 19, 2022

(Going to merge this since it has approval, but happy to continue discussing this!)

@ockham
Copy link
Contributor Author

ockham commented Aug 19, 2022

Failing e2e tests are unrelated to this PR (and also present in other PRs and trunk; they seem related to the Quote block).

@ockham ockham merged commit ccf1f44 into trunk Aug 19, 2022
@ockham ockham deleted the fix/empty-comments-block-wrapper branch August 19, 2022 16:15
@github-actions github-actions bot added this to the Gutenberg 14.0 milestone Aug 19, 2022
@cbravobernal
Copy link
Contributor

Is it possible that Gutenberg hadn't finished rebuilding when you were testing this? (Since this is block PHP, it actually has to go through the build step.)

It could be, but I'm not pretty sure about that. I was using a TwentyTwo with a core/comments-query-loop from previous tests, I checked by editing some styles, so I wasn't on the deprecated comments version.

Anyway, I will be happy to re-test it in better conditions (that WP was really dirty from previous Comments testing) 😓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Comments Affects the Comments Block - formerly known as Comments Query Loop
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comments Query Loop - Empty Markup
3 participants